-
Notifications
You must be signed in to change notification settings - Fork 10.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore(gatsby): convert inference-metadata to typescript #24381
chore(gatsby): convert inference-metadata to typescript #24381
Conversation
state[type] = deleteNode(state[type], { fields: previousFields }) | ||
state[type] = addNode(state[type], { fields: node.fields }) | ||
state[type] = deleteNode(state[type], { ...node, fields: previousFields }) | ||
state[type] = addNode(state[type], { ...node, fields: node.fields }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These 2 changes have the potential to affect production code as we are now sending a full node instead of just fields. I based this on the fact that the action themselves (deleteNode
and addNode
) are already typed and coded to accept a full node as second parameter.
Please let me know if this change is acceptable or how I can write it better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels correct, so it's just a matter of if tests will pass correctly with this change. Thanks for figuring this out!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests seem to pass nicely so it should be all good, I just wanted to make sure this change was seen.
…atsby-inference-metadata
…atsby-inference-metadata
} | ||
} | ||
|
||
module.exports = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be a named export and update the import reference.
e.g.
export function inferenceMetadataReducer(state, action) { .... }
…atsby-inference-metadata
Thank you for the review @blainekasten, I've addressed your feedback, feel free to resolve the conversations if you are ok with the new code ✔️ |
…atsby-inference-metadata
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great to me, let's merge it!
Thank you so much for contributing to our TypeScript refactor! We have more work to do and we would love to have you stay involved in our transition. Please submit more PRs! 💜
Convert
inference-metadata
reducer to TypeScript.Related Issues
#21995